Skip to content

fix(core): Don't require known manager names #2792

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 15, 2025
Merged

fix(core): Don't require known manager names #2792

merged 4 commits into from
Oct 15, 2025

Conversation

@dmihalcik-virtru
Copy link
Member

@dmihalcik-virtru dmihalcik-virtru commented Oct 8, 2025 •
edited
Loading

Proposed Changes

  • The policy service's list of known managers is not exhaustive; that is up to each KAS
  • Adds more logging to help identify misspelled or forgotten mgrs

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@dmihalcik-virtru dmihalcik-virtru requested review from a team as code owners October 8, 2025 13:54
@github-actions github-actions bot added comp:kas Key Access Server size/s labels Oct 8, 2025
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @dmihalcik-virtru, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This PR enhances the KAS service's logging to provide better visibility into registered trust.KeyManagers. It adds an Info level log during service startup to list all registered managers and augments an existing debug log in the DelegatingKeyService to include available manager names when a requested manager is not found. This directly addresses the need for improved debugging of key manager configurations.

Highlights

  • Intent: This pull request aims to improve debugging capabilities by logging the names of registered trust.KeyManagers within the KAS service. This helps identify issues related to misspelled or forgotten key manager configurations.
  • Changes Summary: The changes introduce logging of registered key manager names in two key areas:
  1. KAS Service Registration (service/kas/kas.go): A new slice kmgrNames is introduced to collect the names of all key managers (from KeyManagerFactories, BasicManager, and inProcessService) as they are registered. Once registration is complete, an Info level log message is added to output the full list of registered trust.KeyManagers.
  2. Delegating Key Service (service/trust/delegating_key_service.go): The getKeyManager function now imports maps and slices packages. When a requested key manager factory is not found, an existing debug log message is enhanced to include a list of all currently known key manager names, providing more context for debugging.
  • Reviewer Activity Summary: No specific reviewer activity has been recorded yet for this pull request.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist[bot]
gemini-code-assist bot reviewed Oct 8, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces logging of registered trust.KeyManagers to assist with debugging misspelled or forgotten managers. The changes involve modifying kas.go to collect the names of registered key managers and log them, and updating delegating_key_service.go to include key manager names in debug logs when a key manager factory is not found. I have identified some opportunities to improve the code and have provided suggestions.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 8, 2025

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 174.684899ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 102.532906ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 364.361246ms
Throughput 274.45 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 40.814237647s
Average Latency 405.69644ms
Throughput 122.51 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 28.671534145s
Average Latency 285.7678ms
Throughput 174.39 requests/second

@dmihalcik-virtru dmihalcik-virtru requested a review from a team as a code owner October 15, 2025 16:08
@dmihalcik-virtru dmihalcik-virtru force-pushed the DSPX-1742-better-loglines branch from f4eda02 to 983b2bc Compare October 15, 2025 16:09
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 181.151239ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 104.56686ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 366.913035ms
Throughput 272.54 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.206088243s
Average Latency 380.102188ms
Throughput 130.87 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.837803878s
Average Latency 267.530005ms
Throughput 186.30 requests/second

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 183.48446ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 103.205765ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 364.105979ms
Throughput 274.65 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.804560107s
Average Latency 386.695737ms
Throughput 128.85 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.065852781s
Average Latency 269.667337ms
Throughput 184.73 requests/second

@dmihalcik-virtru dmihalcik-virtru changed the title chore(core): Log names of registered trust.mgrs fix(core): Don't require known manager names Oct 15, 2025
@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 165.604608ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 99.946259ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 362.685998ms
Throughput 275.72 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.223285437s
Average Latency 380.79364ms
Throughput 130.81 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 27.139240069s
Average Latency 270.38705ms
Throughput 184.24 requests/second

@github-actions
Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 164.266638ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 1000
Denied Decision Requests 0
Total Time 109.081915ms

Standard Benchmark Metrics Skipped or Failed

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 355.841137ms
Throughput 281.02 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 38.020453723s
Average Latency 378.719044ms
Throughput 131.51 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 26.992106593s
Average Latency 268.879899ms
Throughput 185.24 requests/second

cshamrick
cshamrick approved these changes Oct 15, 2025
View reviewed changes
Copy link
Contributor

@cshamrick cshamrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dmihalcik-virtru dmihalcik-virtru added this pull request to the merge queue Oct 15, 2025
Merged via the queue into main with commit 8a56a96 Oct 15, 2025
32 checks passed
@dmihalcik-virtru dmihalcik-virtru deleted the DSPX-1742-better-loglines branch October 15, 2025 20:18
@opentdf-automation opentdf-automation bot mentioned this pull request Oct 14, 2025
Merged
@dmihalcik-virtru
Copy link
Member Author

/backport

@dmihalcik-virtru
Copy link
Member Author

/backport

@github-actions github-actions bot added the comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) label Oct 17, 2025
@opentdf-automation
Copy link
Contributor

Successfully created backport PR for release/service/v0.10:

opentdf-automation bot pushed a commit that referenced this pull request Oct 17, 2025
@dmihalcik-virtru @github-actions
fix(core): Don't require known manager names (#2792)
e90916a 
### Proposed Changes

* The policy service's list of known managers is not exhaustive; that is
up to each KAS
* Adds more logging to help identify misspelled or forgotten mgrs

### Checklist

- [ ] I have added or updated unit tests
- [ ] I have added or updated integration tests (if appropriate)
- [ ] I have added or updated documentation

### Testing Instructions

(cherry picked from commit 8a56a96)
@opentdf-automation opentdf-automation bot mentioned this pull request Oct 17, 2025
Merged
opentdf-automation bot added a commit that referenced this pull request Oct 17, 2025
@opentdf-automation
fix(core): Don't require known manager names (#2792)
c06bb3f 
### Proposed Changes

* The policy service's list of known managers is not exhaustive; that is
up to each KAS
* Adds more logging to help identify misspelled or forgotten mgrs

### Checklist

- [ ] I have added or updated unit tests
- [ ] I have added or updated integration tests (if appropriate)
- [ ] I have added or updated documentation

### Testing Instructions

(cherry picked from commit 8a56a96)
dmihalcik-virtru pushed a commit that referenced this pull request Oct 17, 2025
@opentdf-automation
fix(core): Don't require known manager names [backport to release/ser…
cf87419 
…vice/v0.10] (#2815)

# Description
Backport of #2792 to `release/service/v0.10`.

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Oct 22, 2025
@opentdf-automation
chore(main): release service 0.11.0 (#2744)
e353b0b 
🤖 I have created a release *beep* *boop*
---


##
[0.11.0](service/v0.10.0...service/v0.11.0)
(2025-10-22)


### Features

* **authz:** add obligation fulfillment logic to obligation PDP
([#2740](#2740))
([2f8d30d](2f8d30d))
* **authz:** audit logs should properly handle obligations
([#2824](#2824))
([874ec7b](874ec7b))
* **authz:** defer to request auth as decision/entitlements entity
([#2789](#2789))
([feb34d8](feb34d8))
* **authz:** obligations protos within auth service
([#2745](#2745))
([41ee5a8](41ee5a8))
* **authz:** protovalidate tests for new authz obligations fields
([#2747](#2747))
([73e6319](73e6319))
* **authz:** service logic to use request auth as entity identifier in
PDP decisions/entitlements
([#2790](#2790))
([6784e88](6784e88))
* **authz:** wire up obligations enforcement in auth service
([#2756](#2756))
([11b3ea9](11b3ea9))
* **core:** propagate token clientID on configured claim via interceptor
into shared context metadata
([#2760](#2760))
([0f77246](0f77246))
* **kas:** Add required obligations to kao metadata.:
([#2806](#2806))
([16fb26c](16fb26c))
* **policy:** add FQNs to obligation defs + vals
([#2749](#2749))
([fa2585c](fa2585c))
* **policy:** Add obligation support to KAS
([#2786](#2786))
([bb1bca0](bb1bca0))
* **policy:** List obligation triggers rpc
([#2823](#2823))
([206abe3](206abe3))
* **policy:** namespace root certificates
([#2771](#2771))
([beaff21](beaff21))
* **policy:** Proto - root certificates by namespace
([#2800](#2800))
([0edb359](0edb359))
* **policy:** Protos List obligation triggers
([#2803](#2803))
([b32df81](b32df81))
* **policy:** Return built obligations fqns with triggers.
([#2830](#2830))
([e843018](e843018))
* **policy:** Return obligations from GetAttributeValue calls
([#2742](#2742))
([aa9b393](aa9b393))


### Bug Fixes

* **core:** CORS
([#2787](#2787))
([a030ac6](a030ac6))
* **core:** deprecate policy WithValue selector not utilized by RPC
([#2794](#2794))
([c573595](c573595))
* **core:** deprecated stale protos and add better upgrade comments
([#2793](#2793))
([f2678cc](f2678cc))
* **core:** Don't require known manager names
([#2792](#2792))
([8a56a96](8a56a96))
* **core:** Fix mode negation and core mode
([#2779](#2779))
([de9807d](de9807d))
* **core:** resolve environment loading issues
([#2827](#2827))
([9af3184](9af3184))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.6.0 to
0.7.0 in /service
([#2812](#2812))
([a6d180d](a6d180d))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.12.0 to
0.13.0 in /service
([#2814](#2814))
([5e9c695](5e9c695))
* **deps:** bump github.com/opentdf/platform/sdk from 0.7.0 to 0.9.0 in
/service ([#2798](#2798))
([d6bc9a8](d6bc9a8))
* **deps:** bump github.com/opentdf/platform/sdk from 0.9.0 to 0.10.0 in
/service ([#2831](#2831))
([412dfd1](412dfd1))
* ECC key loading (deprecated)
([#2757](#2757))
([49990eb](49990eb))
* **policy:** Change to nil
([#2746](#2746))
([a449434](a449434))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cshamrick cshamrick cshamrick approved these changes

@c-r33d c-r33d c-r33d approved these changes

@sujankota sujankota Awaiting requested review from sujankota

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backport release/service/v0.10 comp:kas Key Access Server comp:policy Policy Configuration ( attributes, subject mappings, resource mappings, kas registry) size/s

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dmihalcik-virtru @cshamrick @c-r33d